Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Types: luma.gl Parameters #9209

Open
wants to merge 11 commits into
base: master
Choose a base branch
from
Open

Conversation

felixpalmer
Copy link
Collaborator

Background

I noticed that that passing depthTest in Parameters raises a type error, despite working in the past:

const scatter = new ScatterplotLayer({
  parameters: {depthTest: false}
});

Even though luma is not including this on the Parameters type, it continues to work in practice.

I've gone ahead and added some types to surface the inconsistencies, and found that the following keys are used by deck but untyped in luma:

  • depthRange (collision-filter-pass.ts)
  • depthTest (terrain-pass.ts)

@ibgreen should these types be added to luma or should deck be migrating to a new API?

Change List

  • Add Parameters type where previously missing
  • Add isDrawableLayerParameters type helper

@ibgreen
Copy link
Collaborator

ibgreen commented Oct 10, 2024

Type errors will occur when using non-WebGPU compatible parameters. Perhaps they work now but if so they will soon no longer work

Docs at https://luma.gl/docs/api-reference/core/parameters
Type defs here https://github.com/visgl/luma.gl/blob/master/modules/core/src/adapter/types/parameters.ts#L99

Recommendations:

@felixpalmer
Copy link
Collaborator Author

Regarding depthRange, it isn't clear why we even need to add this. We only ever set it to [0,1]. I think it originally was introduced as a Mapbox workaround and the usage then spread to other places.

@coveralls
Copy link

Coverage Status

coverage: 91.647% (+0.004%) from 91.643%
when pulling 4b4371d on felix/draw-layer-parameters
into 065816e on master.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants